Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[mqtt.homeassistant] Fix newStyleChannels #17491

Merged
merged 7 commits into from
Oct 7, 2024
Merged

[mqtt.homeassistant] Fix newStyleChannels #17491

merged 7 commits into from
Oct 7, 2024

Conversation

ccutrer
Copy link
Contributor

@ccutrer ccutrer commented Oct 1, 2024 •
edited
Loading

This fixes a couple of problems. The biggest is that if multiple components were single channel OR no-name, they would all get mapped to a "groupId" of "", and the next time the Thing started, all but one of them would disappear unless new discovery information was received. While fixing that, I fixed two other issues:

  • Multi-channel components with a blank name were getting all their channels dumped into the top level of no group. In the newStyleChannels world, we don't care about the component's name anymore - multi-channel components get a group, single-channel components do not.
  • Instead of statically saying that certain components are only ever single or multi-channel components, determine at runtime if a component only added a single channel, and if so re-classify it as a single-channel component, moving its channel out of the group (and just naming it as the group was named)

As part of this, I needed to keep a separate mapping of Channel ID to Channel State, instead of relying on the component ID matching the channel ID. Doing this enabled further simplifying of channel IDs by not including the component type at all in the channel ID, unless it's necessary to resolve a conflict from a duplicate object id of a different component type (which should be rare to never). In practice, this simplifies lots of channels like light_dimmer, switch_switch, cover_cover, lock_lock to just dimmer, switch, cover, and lock.

@ccutrer ccutrer requested a review from antroids as a code owner October 1, 2024 19:43
@ccutrer ccutrer marked this pull request as draft October 1, 2024 20:26
@ccutrer ccutrer force-pushed the mqtt-homeassistant-fix-newStyleChannels branch from b37f60b to 80f6ad7 Compare October 1, 2024 21:27
@ccutrer ccutrer marked this pull request as ready for review October 1, 2024 21:43
@ccutrer ccutrer requested a review from lsiepel October 1, 2024 21:43
@ccutrer
Copy link
Contributor Author

ccutrer commented Oct 1, 2024

While this commit changes channel IDs, it shouldn't be considered breaking because it just further refines another breaking change to channel IDs introduced in 4.3

@lsiepel
Copy link
Contributor

lsiepel commented Oct 2, 2024

While this commit changes channel IDs, it shouldn't be considered breaking because it just further refines another breaking change to channel IDs introduced in 4.3

To what PR do you relate this comment?

@lsiepel lsiepel added the enhancement An enhancement or new feature for an existing add-on label Oct 2, 2024
@ccutrer
Copy link
Contributor Author

ccutrer commented Oct 2, 2024

#16600

@lsiepel
Copy link
Contributor

lsiepel commented Oct 2, 2024 •
edited
Loading

Thanks, that PR was not considered breaking due to:
Existing things will not change their UID.

Before i look at the code i wonder if that statement is still true. Not looking for an alltime backwards compatbility here, but just to be clear on the user impact. Ant to determine if an upgrade notification would be usefull.

@ccutrer
Copy link
Contributor Author

ccutrer commented Oct 2, 2024 •
edited
Loading

Right, I guess that is a bit confusing. So that PR is not-breaking to existing installations. New things created will get the new style channels. This PR "breaks" compatibility with those new style channels. But because that PR has not gone out in a proper release yet, it shouldn't be considered actually breaking. Especially since I finally tracked down why I sometimes have channels missing, and it's due to that PR (i.e. new style channels never fully worked).

@lsiepel
Copy link
Contributor

lsiepel commented Oct 2, 2024

Right, I guess that is a bit confusing. So that PR is not-breaking to existing installations. New things created will get the new style channels. This PR "breaks" compatibility with those new style channels. But because that PR has not gone out in a proper release yet, it shouldn't be considered actually breaking. Especially since I finally tracked down why I sometimes have channels missing, and it's due to that PR (i.e. new style channels never fully worked).

I don't consider it breaking compared to the previous PR as that is still the development branch, but just wanted to be sure that exising (openHAB 4.2.2 or before) installations are not affected.

@ccutrer ccutrer marked this pull request as draft October 2, 2024 20:59
@ccutrer
Copy link
Contributor Author

ccutrer commented Oct 2, 2024

... and this can't be merged yet, either. I just found a bug that some components/channels don't send their updates on properly

@ccutrer ccutrer marked this pull request as ready for review October 2, 2024 22:04
@ccutrer ccutrer added the bug An unexpected problem or unintended behavior of an add-on label Oct 3, 2024
@ccutrer
Copy link
Contributor Author

ccutrer commented Oct 3, 2024

@lsiepel this should be ready now. I'll continue testing over the next few days anyway, but things seem pretty happy and I've moved on to other fixes and enhancements. And I labeled it as a bug - this really needs to get in before 4.3 goes final, otherwise more complex Home Assistant things will be intermittently broken.

lsiepel
lsiepel requested changes Oct 4, 2024
View reviewed changes
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small comments. As you are doing a ton of work to this binding, it would be nice if you could also try to reduce the compiler warnings while doing so. Not asking you to do it all in once, but just keep it in mind.

.../java/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandler.java Outdated Show resolved Hide resolved
.../main/java/org/openhab/binding/mqtt/homeassistant/internal/component/DefaultSchemaLight.java Outdated Show resolved Hide resolved
.../main/java/org/openhab/binding/mqtt/homeassistant/internal/component/DefaultSchemaLight.java Outdated Show resolved Hide resolved
@lsiepel lsiepel removed the enhancement An enhancement or new feature for an existing add-on label Oct 5, 2024
@lsiepel
Copy link
Contributor

lsiepel commented Oct 7, 2024

itest seems to fail.

ccutrer and others added 7 commits October 7, 2024 15:17
@ccutrer
[mqtt.homeassistant] fix newStyleChannels
80b9f6f 
This fixes a couple of problems. The biggest is that if multiple components
were single channel OR no-name, they would all get mapped to a "groupId" of "",
and the next time the Thing started, all but one of them would disappear unless
new discovery information was received. While fixing that, I fixed two other
issues:
 * Multi-channel components with a blank name were getting all their channels
   dumped into the top level of no group. In the newStyleChannels world, we
   don't care about the component's name anymore - multi-channel components
   get a group, single-channel components do not.
 * Instead of statically saying that certain components are only ever single
   or multi-channel components, determine at runtime if a component only added
   a single channel, and if so re-classify it as a single-channel component,
   moving its channel out of the group (and just naming it as the group was
   named)

Signed-off-by: Cody Cutrer <cody@cutrer.us>
@ccutrer
further simplify channel IDs
7617dc3 
don't include the component type in the channel ID unless it's
necessary to resolve a conflicting object ID from another component.
this is now possible since we don't have to rely on the channel ID
matching the component ID to get its channel state

Signed-off-by: Cody Cutrer <cody@cutrer.us>
@ccutrer
Fix state updates not propagating back from MQTT on channels whose UI…
28f888d 
…D changed

Signed-off-by: Cody Cutrer <cody@cutrer.us>
@ccutrer
Fix state updates for components that have additional state processin…
603a0d3 
…g handlers

Only need to adjust for channels that could be renamed due to being single
channel components

Signed-off-by: Cody Cutrer <cody@cutrer.us>
@ccutrer @lsiepel
Apply suggestions from code review
1322461 
Co-authored-by: lsiepel <leosiepel@gmail.com>
Signed-off-by: Cody Cutrer <cody@cutrer.us>
@ccutrer
fix spotless
29d6127 
Signed-off-by: Cody Cutrer <cody@cutrer.us>
@ccutrer
fix itests
10a3259 
Signed-off-by: Cody Cutrer <cody@cutrer.us>
@ccutrer ccutrer force-pushed the mqtt-homeassistant-fix-newStyleChannels branch from 9e6cb7b to 10a3259 Compare October 7, 2024 21:20
@ccutrer
Copy link
Contributor Author

ccutrer commented Oct 7, 2024

fixed (I did have to rebase so that itests compiled correctly; presumably due to #17509

@lsiepel lsiepel added the rebuild Triggers Jenkins PR build label Oct 7, 2024
@lsiepel lsiepel added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Oct 7, 2024
@lsiepel lsiepel merged commit 31f6cda into openhab:main Oct 7, 2024
4 of 5 checks passed
@lsiepel lsiepel added this to the 4.3 milestone Oct 7, 2024
@lsiepel
Copy link
Contributor

lsiepel commented Oct 7, 2024

Thanks, LGTM

@ccutrer ccutrer deleted the mqtt-homeassistant-fix-newStyleChannels branch October 7, 2024 22:09
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
@ccutrer @joni1993
[mqtt.homeassistant] fix newStyleChannels (openhab#17491)
db27ee8 
* [mqtt.homeassistant] fix newStyleChannels
* further simplify channel IDs

Signed-off-by: Cody Cutrer <cody@cutrer.us>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
@ccutrer @matchews
[mqtt.homeassistant] fix newStyleChannels (openhab#17491)
5bf9574 
* [mqtt.homeassistant] fix newStyleChannels
* further simplify channel IDs

Signed-off-by: Cody Cutrer <cody@cutrer.us>
@jlaur jlaur changed the title [mqtt.homeassistant] fix newStyleChannels [mqtt.homeassistant] Fix newStyleChannels Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsiepel lsiepel lsiepel requested changes

@antroids antroids Awaiting requested review from antroids antroids is a code owner

Assignees
No one assigned
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
2 participants
@ccutrer @lsiepel